Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix for attempting to remove a lib when dev is set #644

Open
wants to merge 5 commits into
base: development
Choose a base branch
from
Open

fix for attempting to remove a lib when dev is set #644

wants to merge 5 commits into from

Conversation

ninjamuffin99
Copy link
Contributor

  • run something like `haxelib git [lib] [url]
  • set a devpath somewhere haxelib dev [lib] path
  • try to remove the git haxelib with haxelib remove [lib] git

Error: Library haxelib version git is not installed

currently Repository.removeProjectVersion() doesn't check if we're using a dev path, it only checks the .current file. So here we check if we have a .dev file (using getDevPath() which returns null if no .dev file is found)

This PR just adds a simple check calling getDevPath to see if we are using a dev path, in which case we should be able to remove the git version of a haxelib

@ninjamuffin99
Copy link
Contributor Author

one thing this doesn't accomodate for yet, it should change the .current file right 🤔? right now it doesn't do that... so returning to the non-dev version it would try to use git version.

@kLabz
Copy link
Contributor

kLabz commented Aug 26, 2024

This PR doesn't seem to do what the description says 🤔

The codepath leading to the error you mentioned has not changed. Changing the behavior when removing the version used as "current" might not be what we want either, and implementation seems wrong anyway.

@ninjamuffin99
Copy link
Contributor Author

ninjamuffin99 commented Aug 26, 2024 •
edited
Loading

hrmm I suppose what I'm trying to do is: when using a dev version for a haxelib, and then explicitly asking to haxelib remove a specific version, we should be able to remove the version, since we aren't using it (we are using the dev version`.)

haxelib remove acts as if you're still using a non-dev haxelib, so it will run to see if the "current" set version of a haxelib is the one you want to remove. I feel like this is incorrect behaviour, since it's NOT being used currently, the dev version is after running haxelib dev. However haxelib uses 2 files to track versions

  • .current, whether we should look in a semantic version styled folder for our code, and which one we should use. Can also be set to git
  • .dev, if this file exists, we are using a development version, within is just the full path to the haxelib somewhere on your machine. If this file doesn't exist, we are NOT using a dev directory

So Repository.removeProjectVersion()'s current implementation doesn't care if you have a .dev file or not, if the .current file is set to something you want to remove, it will refuse and error.

Repository.getDevPath() will return null if .dev doesn't exist, so that's the check I used in the if statement

kLabz
kLabz reviewed Aug 26, 2024
View reviewed changes
src/haxelib/api/Repository.hx Outdated
@@ -180,6 +181,9 @@ class Repository {
}

FsUtils.deleteRec(versionPath);

var versions = getProjectInstallationInfo(name).versions.pop();
setCurrentVersion(name, versions);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really don't want that. Especially since you're also doing it when removing a version that is not the current version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I was incorrect with this one yes.
The intention was for the case of using a dev version and if you were to remove the current version, it doesn't change anything in the .current file.

If you're on say flixel 5.0.0 (cat flixel/.current -> 5.0.0), begin using a dev directory for flixel, do something like haxelib remove flixel 5.0.0, we want to properly update .current so if you disable the dev, it leads to whatever the most recent directory is versions.pop().

However you're right, my implementation is incorrect since we don't want to remove / set the current version unconditionally!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does something like

FsUtils.deleteRec(versionPath);
		
if (current == version)
{
	var versions = getProjectInstallationInfo(name).versions.pop();
	setCurrentVersion(name, versions);
}

shake out? Unless I'm still misunderstanding 🙇‍♂️
In this snippet, the code will run if .dev DOES exist (otherwise we would have thrown an error earlier). So if we have a .dev haxelib, and we just removed our .current set haxelib, we update the .current file to the most recent version we have installed. Again, only if we are:

  • uninstalling our "current" version
  • we are using a haxelib dev based haxelib
  • there are other versions installed

@skial skial mentioned this pull request Sep 4, 2024
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kLabz kLabz kLabz left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ninjamuffin99 @kLabz